Add DLM force merge operation helpers#143200
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds helper methods to perform deduplicated DLM force merge operations within the ForceMergeStep class, and removes a test for the step name per prior feedback. The changes move force merge logic from a centralized service pattern to a more distributed step-based pattern, where each step handles its own force merge execution through the DlmStepContext deduplication framework.
Changes:
- Added
maybeForceMergeandforceMergehelper methods to ForceMergeStep for executing deduplicated force merge operations - Made ForceMergeRequestWrapper class and constructor public to allow usage from ForceMergeStep
- Removed extraneous
testStepNametest and added comprehensive tests for the new force merge methods
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ForceMergeStep.java | Added helper methods (maybeForceMerge, forceMerge, formForceMergeRequest) to handle deduplicated force merge execution with error handling and logging |
| DataStreamLifecycleService.java | Changed ForceMergeRequestWrapper visibility from package-private to public to enable usage outside the service class |
| ForceMergeStepTests.java | Removed testStepName test and added comprehensive tests for the new force merge functionality including success, failure, and error recording scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
...test/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStepTests.java
Show resolved
Hide resolved
...treams/src/main/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleService.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
...treams/src/main/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleService.java
Outdated
Show resolved
Hide resolved
...test/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStepTests.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
…ms/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co>
|
Decided to just add the execute() implementation to this one since it was just a line and this PR is still short |
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Show resolved
Hide resolved
...test/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStepTests.java
Outdated
Show resolved
Hide resolved
…ms/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co>
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
...treams/src/main/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
…ms/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
dakrone
left a comment
There was a problem hiding this comment.
I left a comment about handling unhealthy indices.
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java
Outdated
Show resolved
Hide resolved
dakrone
left a comment
There was a problem hiding this comment.
LGTM, thanks for iterating on this Sean!
* Add DLM force merge operation helpers * adapt data lifecycleservice force merge code * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co> * PR feedback * [CI] Auto commit changes from spotless * implement execute(), add completeness check before running maybeForceMerge() * add unit test for when force merge is already complete * add check for nonexistent index * update comment * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co> * [CI] Auto commit changes from spotless * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com> * respond to PR feedback * use unavailable shards method * address PR feedback * adjust to max timeout for force merge --------- Co-authored-by: Luke Whiting <luke.whiting@elastic.co> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
* Add DLM force merge operation helpers * adapt data lifecycleservice force merge code * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co> * PR feedback * [CI] Auto commit changes from spotless * implement execute(), add completeness check before running maybeForceMerge() * add unit test for when force merge is already complete * add check for nonexistent index * update comment * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Luke Whiting <luke.whiting@elastic.co> * [CI] Auto commit changes from spotless * Update modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/transitions/steps/ForceMergeStep.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com> * respond to PR feedback * use unavailable shards method * address PR feedback * adjust to max timeout for force merge --------- Co-authored-by: Luke Whiting <luke.whiting@elastic.co> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
This PR adds some helpers to perform the actual deduplicated DLM force merge, and removes some extraneous testing per feedback on a previous PR
For the most part, this code just adapts the existing logic from data lifecycle service. there is some duplication due to this in order to avoid further refactors